Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include repositories Info associated to the Teams #791

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

jbenaventem
Copy link
Contributor

When I get a github_team I need that the repositories that it has associated are informed. I think that It's very usual that the behaviour of the organization can be replaced with the use of the teams for some enterprise accounts, it's for that reason that I need a get the repositories information associated with the Teams.

  • github/data_source_github_organization_teams.go
  • github/data_source_github_team.go
  • github/util_v4_teams.go
   .....
    "repositories": {
         Type:     schema.TypeList,
         Computed: true,
         Elem:     &schema.Schema{Type: schema.TypeString
    },
    ....

@jbenaventem jbenaventem force-pushed the feature/teams-repositories branch from 247d826 to b43ee38 Compare May 21, 2021 11:15
@jcudit jcudit added this to the v4.11.0 milestone May 22, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks solid, but the tests need a bit of fixing up. On the right track overall though!

Comment on lines 17 to 44
config := fmt.Sprintf(`
resource "github_repository" "repo-test" {
name = "tf-acc-repo-%s"
auto_init = true

}

resource "github_team" "team-test" {
name = "tf-acc-test-team01"
}

resource "github_team_repository" "team-repo-test" {
repository = "${github_repository.repo-test.id}"
team_id = "${github_team.team-test.id}"
}

data "github_team" "example" {
slug = "team-test-01"
}

output "team_repository_name" {
value = data.github_team.example.repositories.0
}

output "team_repository_numbers" {
value = data.github_team.example.repositories.#
}
`, randomID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config := fmt.Sprintf(`
resource "github_repository" "repo-test" {
name = "tf-acc-repo-%s"
auto_init = true
}
resource "github_team" "team-test" {
name = "tf-acc-test-team01"
}
resource "github_team_repository" "team-repo-test" {
repository = "${github_repository.repo-test.id}"
team_id = "${github_team.team-test.id}"
}
data "github_team" "example" {
slug = "team-test-01"
}
output "team_repository_name" {
value = data.github_team.example.repositories.0
}
output "team_repository_numbers" {
value = data.github_team.example.repositories.#
}
`, randomID)
config := fmt.Sprintf(`
resource "github_repository" "repo-test" {
name = "tf-acc-repo-%s"
auto_init = true
}
resource "github_team" "team-test" {
name = "tf-acc-test-team-%[1]s"
}
resource "github_team_repository" "team-repo-test" {
repository = "${github_repository.repo-test.id}"
team_id = "${github_team.team-test.id}"
}
data "github_team" "example" {
slug = github_team.team-test.slug
}
`, randomID)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran into some difficulty with getting tests to pass here. This suggests we remove the output blocks as they do not seem to be used and were leading to errors like:

testing.go:654: Step 0 error: Invalid attribute name: On /var/folders/t1/p9cyhv6s2wg4g8s_x6zkpz_m0000gn/T/tf-test790708991/main.tf line 26: An attribute name is required after a dot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It´s a good point.
I´ll try to improve the test! Thks

"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)

func TestAccGithubTeamRepositorty(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func TestAccGithubTeamRepositorty(t *testing.T) {
func TestAccGithubTeamRepository(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to TestAccGithubTeamRepositories to avoid duplication with test resource_github_team_repository_test.go

`, randomID)

check := resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttrSet("data.github_team.example", "name"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resource.TestCheckResourceAttrSet("data.github_team.example", "name"),
resource.TestCheckResourceAttr("data.github_team.example", "repositories.#", "1"),

A more helpful attribute to check would be the new repositories.# field that this PR adds. When running the test with TF_LOG=DEBUG, I'm seeing expected state:

data.github_team.example:
  ID = 4838313
  provider = provider.github
  description = 
  name = tf-acc-test-team-zniiu
  node_id = MDQ6VGVhbTQ4MzgzMTM=
  permission = pull
  privacy = secret
  repositories.# = 1
  repositories.0 = tf-acc-repo-zniiu
  slug = tf-acc-test-team-zniiu

However, the test fails 😕 . We should find a way to check this attribute as a next step, but I've run out of time for today.

Copy link
Contributor Author

@jbenaventem jbenaventem May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final Change is to use "depends_on". Is a good point for You?

....
                data "github_team" "example" {
			depends_on = ["github_repository.test", "github_team.test", "github_team_repository.test"]
			slug = github_team.test.slug
		  }
		`, randomID)

		check := resource.ComposeAggregateTestCheckFunc(
			resource.TestCheckResourceAttr("data.github_team.example", "repositories.#", "1"),
		)

		testCase := func(t *testing.T, mode string) {
			resource.Test(t, resource.TestCase{
				PreCheck:  func() { skipUnlessMode(t, mode) },
				Providers: testAccProviders,
				Steps: []resource.TestStep{
					{
						Config:             config,
						Check:              check,
						ExpectNonEmptyPlan: true,
					},
				},
			})
		}

@jcudit jcudit modified the milestones: v4.11.0, v4.12.0 Jun 3, 2021
@jcudit jcudit modified the milestones: v4.12.0, v4.13.0 Jun 15, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Suggested a docs change but the test / implementation looks ready to ship 🚀

@@ -30,3 +30,5 @@ data "github_team" "example" {
* `privacy` - the team's privacy type.
* `permission` - the team's permission level.
* `members` - List of team members
* `repositories` - List of team repositories
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get this added to website/docs/d/organization_teams.html.markdown as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!!

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇🏾 for the contribution!

@jcudit jcudit merged commit 6702397 into integrations:master Jun 18, 2021
@robmoss2k
Copy link

This change has made this provider almost unusable for me. I use this data source to look up the team ID from the slug to grant them access to newly created repositories. As the three teams in question have tens of thousands of repositories, terraform now sits there spamming the API until I hit the rate limit. Would it be possible to add a boolean option to this data source stating that you don't want it to read in all the repositories?

@jbenaventem
Copy link
Contributor Author

I think that it´s possible to include that boolean to retrieve the repositories by the team by request.
Its possible that you can give me more context to see as I can help?. If a Team that it has tens of thousands of repositories its possible that this same error you get when try get an Organization too because I suppose that an organization has ten of thousands of repositories too.

Apologies for the problems but I think that it´s important for the provider that it can get this information

@jbenaventem
Copy link
Contributor Author

If you agree with me, we can break the data source into two distinct data sources, one back that it´ll be as the older version and the other that we can retrieve the repositories by team.

@robmoss2k
Copy link

Sorry - I've been away for a few days. I can confirm that I get the same problem with a github_organization data source. Breaking the data source into two distinct data sources, perhaps github_team and github_team_repositories, would be ideal.

@jcudit
Copy link
Contributor

jcudit commented Jun 30, 2021

Happy to release this change once it lands. I agree that a flag to enable the new functionality is a good fit for everyone.

@jbenaventem
Copy link
Contributor Author

I´ll try to contribute it to the next release.
Thx

kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Include repositories Info associated to the Teams

* Improbe Test: depends_on

* update organization_teams markdown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants